-
Notifications
You must be signed in to change notification settings - Fork 1k
feat : added condition to check the RHS of := #6742
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6742 +/- ##
=======================================
Coverage 98.69% 98.69%
=======================================
Files 79 79
Lines 14676 14680 +4
=======================================
+ Hits 14485 14489 +4
Misses 191 191 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tdhock
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also please add test cases based on my issue
This comment was marked as duplicate.
This comment was marked as duplicate.
|
Hi @tdhock,
these changes aim to prevent confusing errors and guide users with actionable suggestions. |
inst/tests/tests.Rraw
Outdated
| test(2318.2, DT[, y := list(mean)], error = "RHS of `:=` is a length-1 list containing a function") | ||
| f = mean | ||
| test(2318.3, DT[, y := f], error = "RHS of `:=` is a function") | ||
| test(2318.4, DT[, y := identity(mean)], error = "RHS of `:=` is a function") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RHS is not a function here
R/data.table.R
Outdated
| stopf("RHS of `:=` is a function. To create a new column of functions or assign to a non-list-column, it must be wrapped in a list(), e.g., `:= list(myfun)`.") | ||
| } | ||
| if (is.list(jval) && length(jval) == 1L && is.function(jval[[1L]]) && nrow(x) > 1L) { | ||
| stopf("RHS of `:=` is a length-1 list containing a function. `data.table` does not automatically recycle lists. To create a list-column, use `rep(list(myfun), .N)`.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is incorrect.
> data.table(x=1:2, L=list(mean))
x L
<int> <list>
1: 1 <function[1]>
2: 2 <function[1]>There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi
i updated the condition and the tests, can you please review if something is missing/wrong ,when you have time.
thank you.
|
Hi @tdhock, @joshhwuu, @Anirban166, Progress update for pr
Prevents malformed data.table objects and confusing errors. At present, this only covers the function case. Other exotic RHS types (e.g., environments, external pointers) are not yet handled. Is there anything else that should be checked or added to this condition? |
|
please resolve conflicts and click Ready for review. |
Closes #5829.
I have added a condition to validate the right-hand side (RHS) of the := operator. If the RHS is a standalone function, an error is raised with an informative message guiding the user to wrap the function in a list if it is intended to be stored.
@tdhock, could you please review this and let me know for further improvements or additional scenarios that I should do?thank you.